Skip to content

Conversation

@maleadt
Copy link
Collaborator

@maleadt maleadt commented Oct 17, 2025

Attempt to simplify the API, after #53 added yet another test-related kwarg.

To be merged, not squashed, as the commits are cleanly separated.

@maleadt maleadt requested a review from vchuravy October 17, 2025 15:44
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this direction.

@maleadt
Copy link
Collaborator Author

maleadt commented Oct 17, 2025

I'll update & check against OpenCL.jl tomorrow.

@vchuravy
Copy link
Member

JuliaGPU/OpenCL.jl@7ac6c7a

@vchuravy
Copy link
Member

One thing we should add a test for: Currently I believe it is possible to filter out a test by default and then run it with the command line interface.

As an example in Enzyme, we currently have:

https://github.com/EnzymeAD/Enzyme.jl/blob/0d9ed777e62ea304138ae6483ddc5f7576c02f1a/test/runtests.jl#L5

But IIRC we should still be able to run our by default inactive test suites since the filtering was either command line or function based.

With the delete! approach one loses that flexibility

@maleadt
Copy link
Collaborator Author

maleadt commented Oct 20, 2025

Alright, that muddies the API up a little, but I think I've made that possible now.
Also see: JuliaGPU/OpenCL.jl@2b22eb9

@vchuravy vchuravy requested a review from giordano October 20, 2025 13:22
Copy link
Collaborator

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a long shot, and mostly tangential to this PR (although the logic for the following may be related to the format of testsuite), but any thoughts on adding ways to for example have some testsets running serially? I think some tests in Julia do that, right? Or customise the order, besides the number of lines of code (for very manual load balancing basically).

@maleadt
Copy link
Collaborator Author

maleadt commented Oct 21, 2025

any thoughts on adding ways to for example have some testsets running serially

Probably a separate serial_tests argument? Or maybe test_worker could also return an integer to indicate which worker to run on?

@maleadt maleadt marked this pull request as ready for review October 22, 2025 10:46
Copy link
Collaborator

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, besides the missing docstring

@maleadt
Copy link
Collaborator Author

maleadt commented Oct 22, 2025

I'll merge this but wait with tagging in case @vchuravy has any more comments (or if he wants to redesign addworker).

@maleadt maleadt merged commit 74fdcb8 into main Oct 22, 2025
20 checks passed
@maleadt maleadt deleted the tb/testsuite branch October 22, 2025 11:55
custom_tests::Dict{String, Expr}=Dict{String, Expr}(), init_code = :(),
test_worker = Returns(nothing), stdout = Base.stdout, stderr = Base.stderr)
function runtests(mod::Module, args::ParsedArgs;
testsuite::Dict{String,Expr} = find_tests(pwd()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pwd() is problematic: JuliaAstro/BackgroundMeshes.jl#19 (comment). I don't see a better alternative though, basically we'd like a call-place @__DIR__. Since we're passing the module, one option could be joinpath(pkgdir(mod), "test"), but that assumes that (1) tests are always in the test subdirectory and that (2) pkgdir(mod) return something sensible, but it can also return nothing in some cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find either assumption unreasonable, and if either doesn't hold the user can provide their own path?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants